feat: parse inscription like witness data #2524
Conversation
bitSmiley
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe update enhances Bitcoin transaction processing by introducing methods for handling witness data and decoding scripts. The new function Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
bitSmiley
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- zetaclient/chains/bitcoin/observer/inbound.go (1 hunks)
- zetaclient/chains/bitcoin/tx_script.go (3 hunks)
- zetaclient/chains/bitcoin/tx_script_test.go (1 hunks)
Additional context used
Path-based instructions (3)
zetaclient/chains/bitcoin/observer/inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/tx_script.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/tx_script_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (3)
zetaclient/chains/bitcoin/observer/inbound.go (1)
476-510: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GetBtcEventWithWitnessare correctly implemented.zetaclient/chains/bitcoin/tx_script.go (1)
340-345: LGTM!The code changes are approved.
zetaclient/chains/bitcoin/tx_script_test.go (1)
495-556: LGTM! Consider adding more edge cases for comprehensive testing.The code changes are approved.
For comprehensive testing, consider adding more edge cases, such as:
- Scripts with unexpected opcodes.
- Scripts with valid but irrelevant data.
- Scripts with nested structures.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
zetaclient/chains/bitcoin/observer/inbound.go (2)
476-479: Extend the function comments.The comments should provide a complete description of the function's purpose and behavior, including the format of the tapscript.
-// The format of the tapscript is +// The format of the tapscript is as follows: +// <description of the tapscript format>
500-501: Improve error handling for clarity.The comment "should never happen" is not informative. Provide a more descriptive error message.
-if err != nil { // should never happen - return nil, errors.Wrap(err, "unable to get btc event") +if err != nil { + return nil, errors.Wrap(err, "failed to get BTC event, unexpected error")
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- zetaclient/chains/bitcoin/observer/inbound.go (1 hunks)
- zetaclient/chains/bitcoin/tx_script.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- zetaclient/chains/bitcoin/tx_script.go
Additional context used
Path-based instructions (1)
zetaclient/chains/bitcoin/observer/inbound.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
|
@lumtis @swift1337 I have addressed the review feedbacks. I'm not familiar with the "resolve" convention you have, so I just leave them open first. Please take another look and let me know. |
bitSmiley
force-pushed
the
feat-btc_inscription
branch
from
7affe31 to
1c7c094
Compare
|
@lumtis Ran |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- zetaclient/chains/bitcoin/observer/inbound.go (1 hunks)
- zetaclient/chains/bitcoin/tokenizer.go (1 hunks)
- zetaclient/chains/bitcoin/tx_script.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- zetaclient/chains/bitcoin/observer/inbound.go
Additional context used
Path-based instructions (2)
zetaclient/chains/bitcoin/tokenizer.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.zetaclient/chains/bitcoin/tx_script.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Learnings (1)
zetaclient/chains/bitcoin/tx_script.go (1)
Learnt from: lumtis PR: zeta-chain/node#2524 File: zetaclient/chains/bitcoin/tx_script.go:280-324 Timestamp: 2024-07-22T10:48:05.549Z Learning: Refactor loop structures in Go code for better readability by using the `for t.Next()` pattern instead of a `for { if !t.Next() { ... } }` pattern where applicable.
Additional comments not posted (10)
zetaclient/chains/bitcoin/tokenizer.go (7)
10-15: LGTM!The function correctly initializes the
scriptTokenizerstruct.
21-27: LGTM!The
scriptTokenizerstruct is well-defined and includes necessary fields for script tokenization.
31-33: LGTM!The
Donefunction correctly checks if the tokenizer has completed processing the script.
37-39: LGTM!The
Datafunction correctly returns the data associated with the most recently parsed opcode.
43-45: LGTM!The
Errfunction correctly returns any errors associated with the tokenizer.
48-50: LGTM!The
Opcodefunction correctly returns the current opcode associated with the tokenizer.
68-155: LGTM! Consider adding more context to error messages.The
Nextfunction correctly handles multiple cases for different opcodes and includes error handling.For better debugging, consider adding more context to the error messages.
- t.err = fmt.Errorf("opcode %d requires %d bytes, but script only has %d remaining", op, length, len(script)) + t.err = fmt.Errorf("Next: opcode %d requires %d bytes, but script only has %d remaining", op, length, len(script)) - t.err = fmt.Errorf("unexpected op code %d", op) + t.err = fmt.Errorf("Next: unexpected op code %d", op) - t.err = fmt.Errorf("opcode %d pushes %d bytes, but script only has %d remaining", op, dataLen, len(script)) + t.err = fmt.Errorf("Next: opcode %d pushes %d bytes, but script only has %d remaining", op, dataLen, len(script)) - t.err = fmt.Errorf("invalid opcode length %d", length) + t.err = fmt.Errorf("Next: invalid opcode length %d", length)zetaclient/chains/bitcoin/tx_script.go (3)
210-223: LGTM! Consider adding more context to error messages.The
DecodeScriptfunction is well-structured and correctly decodes the memo wrapped in an inscription-like script.For better debugging, consider adding more context to the error messages.
- return nil, false, errors.Wrap(err, "checkInscriptionEnvelope: unable to check the envelope") + return nil, false, errors.Wrap(err, "DecodeScript: checkInscriptionEnvelope: unable to check the envelope") - return nil, false, errors.Wrap(err, "decodeInscriptionPayload: unable to decode the payload") + return nil, false, errors.Wrap(err, "DecodeScript: decodeInscriptionPayload: unable to decode the payload")
279-304: LGTM! Consider adding more context to error messages and refactoring the loop structure.The
decodeInscriptionPayloadfunction is well-structured and correctly processes the script.For better debugging, consider adding more context to the error messages.
- return nil, fmt.Errorf("OP_FALSE not found") + return nil, fmt.Errorf("decodeInscriptionPayload: OP_FALSE not found") - return nil, fmt.Errorf("OP_IF not found") + return nil, fmt.Errorf("decodeInscriptionPayload: OP_IF not found") - return nil, fmt.Errorf("expecting data push, found %d", next) + return nil, fmt.Errorf("decodeInscriptionPayload: expecting data push, found %d", next) - return nil, fmt.Errorf("should contain more data, but script ended") + return nil, fmt.Errorf("decodeInscriptionPayload: should contain more data, but script ended")For better readability, consider refactoring the loop structure.
- for t.Next() { - next = t.Opcode() - if next == txscript.OP_ENDIF { - return memo, nil - } - if next < txscript.OP_DATA_1 || next > txscript.OP_PUSHDATA4 { - return nil, fmt.Errorf("decodeInscriptionPayload: expecting data push, found %d", next) - } - memo = append(memo, t.Data()...) - } - if t.Err() != nil { - return nil, t.Err() - } - return nil, fmt.Errorf("decodeInscriptionPayload: should contain more data, but script ended") + for t.Next() { + next = t.Opcode() + if next == txscript.OP_ENDIF { + return memo, nil + } + if next < txscript.OP_DATA_1 || next > txscript.OP_PUSHDATA4 { + return nil, fmt.Errorf("decodeInscriptionPayload: expecting data push, found %d", next) + } + memo = append(memo, t.Data()...) + } + if t.Err() != nil { + return nil, t.Err() + } + return nil, fmt.Errorf("decodeInscriptionPayload: should contain more data, but script ended")
308-317: LGTM! Consider adding more context to error messages.The
checkInscriptionEnvelopefunction is well-structured and correctly checks the opcodes.For better debugging, consider adding more context to the error messages.
- return fmt.Errorf("cannot obtain public key bytes op %d or err %s", t.Opcode(), t.Err()) + return fmt.Errorf("checkInscriptionEnvelope: cannot obtain public key bytes op %d or err %s", t.Opcode(), t.Err()) - return fmt.Errorf("cannot parse OP_CHECKSIG, op %d or err %s", t.Opcode(), t.Err()) + return fmt.Errorf("checkInscriptionEnvelope: cannot parse OP_CHECKSIG, op %d or err %s", t.Opcode(), t.Err())
|
@fbac I have updated based on your feedbacks. Please help take a look. |
Thank you for the changes, it looks good to me now. Could you fix the two errors concatenating strings and expand the comment where we add 1? After that this can be merged! |
|
@fbac Updated so. Funny thing, seems like my IDE added that concatenation, fix that already. |
Co-authored-by: Dmitry S <[email protected]>
Co-authored-by: Dmitry S <[email protected]>
Co-authored-by: Dmitry S <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Dmitry S <[email protected]>
Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]>
fbac
force-pushed
the
feat-btc_inscription
branch
from
403ad80 to
abf043d
Compare
|
Rebasing the branch with main, if we pass all the checks this will be merged. Also @bitSmiley could you update the changelog introducing this change? |
* parse inscription like witness data * more comment * remove unused code * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * pull origin * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * review feedbacks * update review feedbacks * update make generate * fix linter * remove over flow * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * update review feedback * update code commnet * update comment * more comments * Update changelog.md --------- Co-authored-by: Dmitry S <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> fix version
* feat: parse inscription like witness data (#2524) * parse inscription like witness data * more comment * remove unused code * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * pull origin * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * review feedbacks * update review feedbacks * update make generate * fix linter * remove over flow * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * update review feedback * update code commnet * update comment * more comments * Update changelog.md --------- Co-authored-by: Dmitry S <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> fix version * feat: detect memo in btc txn from OP_RETURN and inscription (#2533) * parse inscription like witness data * more comment * remove unused code * parse inscription * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * pull origin * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * review feedbacks * update review feedbacks * add mainnet txn * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * parse inscription like witness data * more comment * remove unused code * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: Dmitry S <[email protected]> * Update zetaclient/chains/bitcoin/tx_script.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * pull origin * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Dmitry S <[email protected]> * review feedbacks * update review feedbacks * update make generate * fix linter * remove over flow * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/tokenizer.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * update review feedback * update code commnet * update comment * more comments * Update changelog.md * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * Update zetaclient/chains/bitcoin/observer/inbound.go Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * clean up * format code --------- Co-authored-by: Dmitry S <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> * refactor(zetaclient)!: improve AppContext (#2568) * Implement chain registry * Rewrite test-cases for AppContext * Drop `supplychecker` * Refactor app ctx Update worker * Refactor orchestrator * Refactor observer&signer; DROP postBlockHeaders * Fix test cases [1] * Update changelog * Allow Zeta Chain in appContext; address PR comments [1] * Fix app context update * Check for `chain.IsZeta()` * Add AppContext.FilterChains * Fix test cases [2] * Fix test cases [3] * Address PR comments [1] * Address PR comments [2] * Add tests for `slices` * Fix e2e tests [1] * Fix e2e tests [2] * Resolve conflicts, converge codebase between PRs * Add lodash; remove slices pkg * Address PR comments * Minor logging fix * Address PR comments tmp * feat(zetaclient): add generic rpc metrics (#2597) * feat(zetaclient): add generic rpc metrics * feedback * changelog * fmt * fix(zetaclient): use name in pending tx metric (#2642) * feat(pkg): add `ticker` package (#2617) * Add `pkg/ticker` * Sample ticker usage in evm observer * Change naming * Address PR comments * Address PR comments * feat(zetaclient)!: Add support for EIP-1559 gas fees (#2634) * Add Gas struct * Add EIP-1559 fees * Update changelog * Add test cases for legacy vs dynamicFee txs * Fix typo; Add E2E coverage * Address PR comments * Address PR comments * Use gasFeeCap formula * Revert "Use gasFeeCap formula" This reverts commit 2260925. * Address PR comments * Fix e2e upgrade tests * fix: adjust evm outbound tracker reporter to avoid submitting invalid hashes (#2628) * refactor and fix evm outbound tracker reporter to avoid invalid hashes; print log when outbound tracker is full of invalid hashes * add changelog entry * used predefined log fields * remove repeated fields information from log message; Devops team would configure Datadog to show the fields * remove redundant fields in log message; unified logs * remove pending transaction map from observer; the outbound tracker reporter will no longer report pending hash * use bg.Work() to launch outbound tracker reporter goroutines * bring the checking EnsureNoTrackers() back * add more rationale to EVM outbound tracker submission * sync observer and signers without wait on startup * try fixing tss migration E2E failure by increase timeout * feat: Solana relayer (fee payer) key importer, encryption and decryption (#2673) * configure observer relayer key for Solana; remove hardcoded solana test key from zetaclient code * implementation of relayer key importer, encryption and decryption * integrate relayer key into E2E and Solana signer * add relayer_key_balance metrics and unit tests * use TrimSpace to trim password * add changelog entry * use relayer account array in E2E config; a few renaming; add private key validation when importing * fix linter * remove GetNetworkName method for simplification * added PromptPassword method to prompt single password * use network name as map index to store relayer key passwords * moved relayer passwords to chain registry * airdrop SOL token only if solana local node is available --------- Co-authored-by: Lucas Bertrand <[email protected]> * ci: Set Docker Workflow to use Go 1.22 (#2722) * Set go 1.22.2 * Set go 1.22.2 * Set go 1.22 * Set go 1.22 * Refactor contrib/rpc and contrib/docker-scripts to use snapshots API (#2724) Co-authored-by: Julian Rubino <[email protected]> --------- Co-authored-by: dev-bitSmiley <[email protected]> Co-authored-by: Dmitry S <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Francisco de Borja Aranda Castillejo <[email protected]> Co-authored-by: Charlie Chen <[email protected]> Co-authored-by: Lucas Bertrand <[email protected]> Co-authored-by: Charlie <[email protected]> Co-authored-by: Julian Rubino <[email protected]> Co-authored-by: Julian Rubino <[email protected]>
Description
This is the start of a series of PRs that implements the solution mentioned in #2478. The implementation is broken down into several parts:
This PR adds a
DecodeScriptfunction that decodes memo wrapped in an inscription-like script. The format of the script is following that of "inscription" defined in ordinal theory. However, to separate from inscription (as this use case is not an NFT), simplifications are made. The bitcoin envelope script is as follows:The public key is the
internalKeyused in the taproot transaction. This followsord.The
DecodeScriptmethod take the script bytes and parse them according. Internally it is utilizingScriptTokenizer. Ideally, it should directly usetxscript.ScriptTokenizer, however, the current codebase does not have the versions forScriptTokenizer. A simplified version is used. Ifbtcsuiteis upgraded, thenscriptTokenizercan be removed and the code base will be simplified further.// There are no content-type or any other attributes, it's just raw bytes.
How Has This Been Tested?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests